Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: cleanup repository before starting up IPFS #722

Merged
merged 21 commits into from
Dec 8, 2018
Merged

feat: cleanup repository before starting up IPFS #722

merged 21 commits into from
Dec 8, 2018

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Nov 28, 2018

If api file exists and it is the same as the api address defined in the config file, it is removed.

If api file exists and it is different from the one in the config file, the user will be asked if he wants to remove it or not.

Nothing os this will happen if there is a repo.lock.

License: MIT
Signed-off-by: Henrique Dias hacdias@gmail.com

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@ghost ghost assigned hacdias Nov 28, 2018
@ghost ghost added the in progress label Nov 28, 2018
@hacdias hacdias changed the title [WIP] feat: cleanup repository before starting up IPFS feat: cleanup repository before starting up IPFS Nov 28, 2018
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hacdias I think #724 may be a regression introduced by #711 and should be tackled as a part of this PR.

Basically, we need to detect and give user an option to cleanup after hard kill.
It is important because this will happen for regular users that don't play with api but it remains in place when someone kills daemon or PC has a force-shutdown eg. due to power outrage.

  • api and repo.lock are present but no daemon running AND api points at the same multiaddr as config
    • CURRENT behaviour: fails with Error: connect ECONNREFUSED 127.0.0.1:5001
    • TODO: Dialog to user about daemon being offline and give two options:
      • Run ipfs repo fsck and restart IPFS Desktop
      • Quit

Instead of doing manual cleanup, please take a look at re-using CLI command below.
Delegating cleanup to this command should be safer than removing locks by hand, as don't miss things like datastore/LOCK.

ipfs repo fsck

FYI there is a handy command for removing all locks:

'ipfs repo fsck' is a plumbing command that will remove repo and level db
lockfiles, as well as the api file. This command can only run when no ipfs
daemons are running.

It removes:

$IPFS_PATH/repo.lock
$IPFS_PATH/datastore/LOCK
$IPFS_PATH/api

@olizilla
Copy link
Member

@lidel Do we have to prompt the user if we can tell it's from an unclean shutdown? I guess it's safer at this point to draw attention to it rather than hide it.

@lidel
Copy link
Member

lidel commented Nov 29, 2018

@olizilla I think prompt with two options (ipfs repo fsck or quit) is both safe and removes surprises.

It creates an opportunity to backup $IPFS_PATH before running ipfs repo fsck or to quit without doing anything and buys time to figure out what went wrong with <insert super custom setup user created to own demise> :)

@hacdias
Copy link
Member Author

hacdias commented Nov 30, 2018

I can do the dialog as @lidel mentioned, it won't require many changes. It would be nice if js-ipfs-api supported that command. I'll need to run it directly.

@olizilla
Copy link
Member

olizilla commented Dec 3, 2018

@hacdias we have the power to add commands to the http api! Raise an issue on https://github.com/ipfs/js-ipfs-http-client (new name for js-ipfs-api) and explain the use case, so we can get feedback and sanity check if there is any reason not to add it.

@hacdias
Copy link
Member Author

hacdias commented Dec 3, 2018

@lidel
Copy link
Member

lidel commented Dec 3, 2018

Keep in mind we are talking about cleaning up lockfiles BEFORE go-ipfs starts, so http API can't be used anyway and running CLI command directly (with $IPFS_PATH set in env) is the only way to do it :)

@hacdias
Copy link
Member Author

hacdias commented Dec 3, 2018

@olizilla @lidel so... the coded solution works, but I don't like it though. But it works. Could you try?

src/utils/daemon.js Outdated Show resolved Hide resolved
src/utils/daemon.js Outdated Show resolved Hide resolved
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested with api and repo.lock and they are deleted as expected:

$ inotifywait -m --format "%e %f" $IPFS_PATH
(...)
OPEN api
ACCESS api
CLOSE_NOWRITE,CLOSE api
MODIFY repo.lock
OPEN repo.lock
DELETE repo.lock
CLOSE_WRITE,CLOSE repo.lock

Digression: line break after try to looks odd:
2018-12-03--17-52-09

ps. If I click "No", then I get the second error window:
2018-12-03--17-52-51

I wonder if we should look into replacing "No" with "Quit" to avoid jumping between multiple error messages. More than one error dialog feels like a bad UX.

@hacdias
Copy link
Member Author

hacdias commented Dec 3, 2018

@lidel I'm aware. I've a commit I didn't push yet, waiting for more feedback so we can close this soon.

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better!

Note . landed on the new line, probably because endline was read from api file:

2018-12-04--11-30-05

Small fixes suggested below.

src/utils/errors.js Outdated Show resolved Hide resolved
src/utils/daemon.js Outdated Show resolved Hide resolved
src/utils/store.js Outdated Show resolved Hide resolved
src/utils/store.js Outdated Show resolved Hide resolved
const apiFile = join(path, 'api')

if (!await fs.pathExists(apiFile)) {
return true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make the api for cleanup clearer. Having cleanup return true if it doens nothing is suprising. I think we need more info than just a boolean return value.

There are few other issues

  • const cfg = await fs.readJSON(cfgFile) could throw if the config isn't valid json. This function should be robust and try it's best to detect what's going on.
  • if (addr === cfgAddr && (lockFile || datastoreLock)) { I think the situatuon we're looking for is just "there is an api file and no lockFile and no datastroreLock.", then it's safe to just run fsck. If there is a lockfile and an apiFile, but we failed to connect to the api, it'd be ok to ask the user if we should try and fsck. We shouldn't offer to fsck the repo if there is alredy a daemon running that we just can't connect to for some reason.

Let's pull this function out to a helper detectOrphanApiFile that returns true if the api File is present and the same as the config default value, and that there is no repo.lock and no datastore/LOCK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • api and repo.lock are present but no daemon running AND api points at the same multiaddr as config

@lidel is it possible to assert that no daemon is running? is an ECONNREFUSED from the api enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say ECONNREFUSED its the best platform-agnostic hint we can get.

Alternative is to execute platform-specific process enumeration(tasklist on windows, ps -A on linux, ps something on mac) to see if process is running, which may get blocked and not work reliably in more locked down systems.

@olizilla
Copy link
Member

olizilla commented Dec 4, 2018

@Stebalien can we get a sanity check here please.

IPFS Desktop bundles go-ipfs and uses ipfsd-ctl. If the user also has go-ipfs installed manually and the deamon isn't allowed to shutdown cleanly, we've been seeing api files left in the repo, which leaves ipfsd-ctl trying to connect to a running ipfs daemon rather than spawning a new one. This PR is to implement a quick check for where we find an api file in the IPFS_HOME dir, to remove it by calling out to ipfs repo fsk. Is that a reasonable thing to do here?

I wasn't aware of the api file until we hit this issue. @lidel mentioned that he's found examples of people manually editing it as part or elborate schemes to access apis on remote machines.

@Stebalien
Copy link
Member

So, I actually do this (and I know that others do as well). That is, I have an api file pointing to a daemon running as a different (restricted) user.

I'm not sure the best way to fix this. I guess one way is to check for a config file. That is:

  1. If there's no config file, we have a "remote" daemon and should fail.
  2. If there's a config file, we expect a local daemon.
  3. Check if there's an open lock on repo.lock. If so, fail (there's a running daemon).
  4. Otherwise, start the daemon anyways. DO NOT remove the api file, that's racy. Just start the daemon and let it take the repo lock. It should (I think) remove the api file itself.

Alternatively, instead of checking for config, we could check for version. Thoughts?

@olizilla
Copy link
Member

olizilla commented Dec 6, 2018

Thanks @Stebalien. We're going to keep it simple for this pass. If ipfsd-ctl tries to connect to an existing api and fails, and there is an api file present, then we'll offer the user something like:

failed to start dialog

on the assumption that users who are setting api files directly will have a clue as to what is going on, while users that just have an unclean repo state have an option to fsck it, which is a command they may not have been aware of, and it may fix it for them.


if (!origins.includes('webui://-')) origins.push('webui://-')
if (!origins.includes('https://webui.ipfs.io')) origins.push('https://webui.ipfs.io')
if (!showRepoApiFileErrorMessage(ipfsd.repoPath, ipfsd.apiAddr)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is neat, but it's now assuming that there is an api file rather than checking for it. Either we need to check for it so we can assert it in the error message, or we need to update the wording of the error message to something more like:

"IPFS Desktop failed to connect to an existing ipfs api at ${ipfsd.apiAddr}. This can happen if you run ipfs daemon manually and it is not shutdown cleanly. Would you like to try running ipfs repo fsck to remove the lock and api files from ${ipfsd.repoPath} and try again?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just updated the error message.

src/utils/daemon.js Outdated Show resolved Hide resolved

export default async function (opts) {
const ipfsd = await spawn(opts)
await configure(ipfsd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ipfsd connects to an existing daemon process, and we change it's config, we would need to restart that process for the config to be applied.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olizilla we're now editing the config file directly so we won't even be able to change the config of remote daemons. Perhaps it would be better to revert this change and use the API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @olizilla

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olizilla I'm merging this so we can speed up dev. This piece of code will be removed on #736 so let's not take too much time here.

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants